This was from the spec:

The remainder, v % W, must be positive. For languages where % can produce negative values, the correct truncate function is: v - (((v % W) + W) % W)

I don't think we need to use int128_t since W is alway a int32_t and greater than 0.

Yep, the formula is from the spec. The issue is not the spec math, but evaluating it with signed C++ integer types. For int32_t, (v % W) + W can overflow when W is large. For int64_t, the final v - remainder can overflow at INT64_MIN. The wider type keeps the spec formula while avoiding signed UB.

This seems deviate from the Java implementation with some very large values. Perhaps it is worth raising a discussion in the dev@iceberg.apache.org?

I tried this on Compiler Explorer. For W > INT32_MAX / 2, (v % W) + W can overflow, causing the remainder to become negative, which violates the spec that the remainder must be positive.

For values close to std::numeric_limits<int64_t>::min(), our current implementation produces the same result as the modified version, but it triggers

runtime error: signed integer overflow

when compiled with -fsanitize=undefined.

I think Java has a similar issue with the first int32_t overflow case due to wraparound arithmetic, no idea if it suffers from the signed integer overflow problem that C++ has(maybe not).

If we change this, I'd suggest keep v - (((v % W) + W) % W) as a comment and clarify why the current form.

I agree this is worth discussing on the dev mailing list.

I am not very experienced about this process, how do we proceed?

You may want to check https://lists.apache.org/list.html?dev@iceberg.apache.org.